feat: MTK Jacobian for CoupledDEs #229
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
==========================================
+ Coverage 82.00% 86.43% +4.42%
==========================================
Files 15 17 +2
Lines 717 936 +219
==========================================
+ Hits 588 809 +221
+ Misses 129 127 -2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -9,4 +9,4 @@ which are the core systems whose dynamic rule `f` is known analytically. | |||
| This type is used for deciding whether a creation of a [`TangentDynamicalSystem`](@ref) | |||
| is possible or not. | |||
| """ | |||
| CoreDynamicalSystem{IIP} = Union{CoupledODEs{IIP}, DeterministicIteratedMap{IIP}} | |||
| CoreDynamicalSystem{IIP} = Union{CoupledSDEs{IIP}, CoupledODEs{IIP}, DeterministicIteratedMap{IIP}} | |||
There was a problem hiding this comment.
Why is this change made? SDEs are not a core system. Tangent space makes no sense for SDEs, and the tangent space is the only reason for the "Core" hierarchy.
There was a problem hiding this comment.
I just want Jacobian to work for CoupledSDEs, I could also define jacobian(Union{CoupledSDEs, CoreDynamicalSystem})
There was a problem hiding this comment.
It is better to define jacobian(sde::CoupledSDEs) = jacobian(CoupledODEs(sde)). Which is also more transparent. It makes it clear that you want the jacobian of the drift, and not of the noise function g which also has a jacobian.
| @test ode.integ.f.jac([1.0, 1.0], [3.0], 0.0) isa Matrix{Float64} |
There was a problem hiding this comment.
Tests must ONLY use the public API. Not access internal fields that may or may not exist. You have to re-write the tests to use the jacobian function exclusively after you create the ode type.
| @test jac isa Matrix{Num} |
There was a problem hiding this comment.
What is this test testing? calculate_jacobian comes from what package?
There was a problem hiding this comment.
Then we shouldn't be testing it. We should only test functionality from our package. This is easier to maintain and also less confusing. So, the jacobian function and the matrix it gives, which is always a matrix of real numbers.
If you think this function from MTK needs additional testing it is much better to contribute these tests directly to MTK (although I doubt it).
|
|
||
| jac = jacobian(ode) | ||
| @test jac.jac_oop isa RuntimeGeneratedFunction |
There was a problem hiding this comment.
Generally speaking testing the type of things is not of much value. We test functionality and interfaces. Instead of testing whether something "is what it should be", test instead that it "does what it should do".
| jac = jacobian(ode) | ||
| @test jac.jac_oop isa RuntimeGeneratedFunction | ||
| @test jac([1.0, 1.0], [3.0], 0.0) isa Matrix{Float64} |
There was a problem hiding this comment.
ALl of these tests should instead test explicitly the analytic jacobian. That the matrix we get is exactly, numerically, the matrix we expect.
|
sorry for the late reply! |
resolves #228